Repository navigation
Retry transient PyPI failures in metadata lookups - #239
Conversation
metadata.py fetched PyPI JSON and sdists with bare requests.get(), so a single dropped connection failed the run. typeshed CI hit this on test_build_wheel[Authlib] with SSLEOFError while fetching https://pypi.org/pypi/Authlib/json. Move the retry policy that get_version.py already used into stub_uploader/pypi.py and use it for every PyPI request. After the last retry the final response is returned instead of raised, so callers keep reporting the status themselves.
| archive_path = Path(tmpdir, sdist_data["filename"]) | ||
|
|
||
| resp = requests.get(sdist_data["url"], stream=True) | ||
| resp = pypi_get(sdist_data["url"], stream=True, timeout=None) |
There was a problem hiding this comment.
Could we use a generous finite timeout here? The read timeout measures inactivity, not total download duration, so large downloads don't require timeout=None.
| # retried. After the last retry the final response is returned rather than | ||
| # raised, so callers still see the status code and report it themselves. | ||
| retry_strategy = Retry( | ||
| total=RETRIES, status_forcelist=RETRY_ON, raise_on_status=False |
There was a problem hiding this comment.
Could we add a small backoff, e.g. backoff_factor=0.2, so a brief outage doesn’t exhaust all retries immediately? Retry-After is already respected, but isn’t always present.
|
Thanks, both done in 776223b:
The tests now record urllib3's sleeps instead of waiting for them, so the suite stays fast, and one test checks the backoff sequence. Another checks that the sdist download passes the finite timeout. |
srittau
left a comment
srittau
left a comment
There was a problem hiding this comment.
It makes sense to me to extract the fetching logic and retry more aggressively.
We don't need extensive tests that basically just test that requests works as expected, i.e. I don't think we need the test_pypi.py file.
| session = requests.Session() | ||
| adapter = HTTPAdapter(max_retries=retry_strategy) | ||
| session.mount("https://", adapter) | ||
| session.mount("http://", adapter) |
There was a problem hiding this comment.
I hope we don't do any fetching of http resources. Let's remove this adapter. (I think it was needed for the tests.)
Review feedback: nothing fetches http:// resources, and the tests mostly exercised requests itself.
|
Thanks, both done in f3b0550: the |
typeshed's "stub uploader: tests" job failed on python/typeshed#16623 with a network error unrelated to that change:
The failing frame is
metadata.py:423, therequests.get(f"{project_url}/json")inget_upstream_release_metadata.get_version.fetch_pypi_versionsalready retries through aSessionwith a urllib3Retry, but the threerequests.get()calls inmetadata.pyhave no retry and no timeout. One TLS hiccup in any of the ~675test_build_wheelfetches fails the job.This moves that retry policy into
stub_uploader/pypi.pyaspypi_get()and uses it for all four PyPI requests:get_versiontoday:Retry(total=5, status_forcelist=[429, 500, 502, 503, 504])with a 3-second timeout. urllib3 counts connection and TLS errors againsttotal, so anSSLEOFErrorlike the one above is retried.raise_on_status=False: after the last retry the final response comes back rather than aRetryError.validate_pypi_response()therefore still reportsgot 503as before (test_pypi_errorcovers this), andfetch_pypi_versionsstill raises its own error.Session, so connections are reused across lookups. This also matters for the sdist download, which streams (stream=True, thenresp.raw.read()) and needs the pool to stay open afterget()returns. That call keepstimeout=None, as before, so a large sdist isn't cut off.Tests:
tests/test_pypi.pyruns against a local HTTP server: a 503 that recovers, a dropped connection that recovers, retries running out (the 503 is returned afterRETRIES + 1requests), and the shared session. Withtotal=0, the three retry tests fail.test_release_metadata.pynow patchesstub_uploader.metadata.pypi_getinstead ofrequests.get, with the same call assertions.mypy --strict -p stub_uploader -p testsand black 26.5.1 are clean.python -Werror -m pytest testsagainst typeshedmain: 679 passed, including the live PyPI integration tests.